Test shared library on Windows #186

Danielius1922 · 2022-02-11T09:54:00Z

No description provided.

Danielius1922 · 2022-02-11T11:27:39Z

This is POC for solving issue #182. I've created a small shared library (uuid-shared) and test application (uuid-app), uuid-app links with uuid-shared and it compiles successfully on both Linux and Windows.
If the naming and location are fine then we can merge the oc_export.h header. Though going through all header files and adding the annotation will take a while. Maybe we can do it by parts somehow?

WAvdBeek · 2022-02-11T11:42:03Z

This is POC for solving issue #182. I've created a small shared library (uuid-shared) and test application (uuid-app), uuid-app links with uuid-shared and it compiles successfully on both Linux and Windows. If the naming and location are fine then we can merge the oc_export.h header. Though going through all header files and adding the annotation will take a while. Maybe we can do it by parts somehow?

Note that from python we are just using the oc_python.c file... so if we have an header file for that code, then one can build C# like the python code. C# has a similar interface as python c_types, e.g. add the call format to the dll in the C# code.
https://www.nag.com/IndustryArticles/Calling_C_Library_DLLs_from_C_Sharp.pdf

Danielius1922 · 2022-02-11T15:46:28Z

Note that from python we are just using the oc_python.c file... so if we have an header file for that code, then one can build C# like the python code. C# has a similar interface as python c_types, e.g. add the call format to the dll in the C# code. https://www.nag.com/IndustryArticles/Calling_C_Library_DLLs_from_C_Sharp.pdf

You mean adding oc_python.h with the OC_API annotation for non-static functions or am I misunderstanding?

WAvdBeek · 2022-02-11T15:49:52Z

Note that from python we are just using the oc_python.c file... so if we have an header file for that code, then one can build C# like the python code. C# has a similar interface as python c_types, e.g. add the call format to the dll in the C# code. https://www.nag.com/IndustryArticles/Calling_C_Library_DLLs_from_C_Sharp.pdf

You mean adding oc_python.h with the OC_API annotation for non-static functions or am I misunderstanding?

correct. then we can also run the python (client) code on windows. not sure if we want to do the same API as JAVA, that is a considerable amount of work.

Danielius1922 · 2022-02-11T16:12:59Z

correct. then we can also run the python (client) code on windows. not sure if we want to do the same API as JAVA, that is a considerable amount of work.

Sure, no problem, I can do it, shouldn't take that long to do the Windows part.

include/oc_export.h

Danielius1922 · 2022-02-11T17:12:55Z

CMakeLists.txt

+)
+
+# Python client
+if(UNIX OR WIN32)
    set(client-python-lib-obj


Hmm, I don't think I can reuse the .o files. I need different annotations when I compile for static or shared lib.

@Oxymoron79 I guess the solution here is to create shared version of the object libraries (common-obj and common-obj-shared, etc.), since they have differing compile definitions. Though they differ only in the compile definitions, so the CMake code for both targets will be very similar. I should probably create a CMake function that creates a library (both static and shared) target to avoid most of the duplicity.

Is that really necessary? The point of the object libraries is to generate static and dynamic libraries without the need to recompile the source...

Is your reasoning for this the declspec(dllexport) annotation for Windows? If so, I doubt that different object files are needed, because the user of the DLL has to change the annotation to declspec(dllimport) when linking to the binary (which was compiled with declspec(dllexport)). But my knowledge of Windows development is limited - so take that with a grain of salt 😉

I would like avoid the duplicity if possible, but when I compile libiotivity-lite-client-python.so.2.2.5 and examine the public symbols using nm -D command - I see that all global functions from common-obj, tinycbor-master, client-obj and python-obj are public. Thus, adding C_VISIBILITY to the final shared library target had no effect. It seems that I have to compile the .o files with the macro definitions for a shared library, linking is not enough. However, I haven't tried it the other way around, add definitions for shared library to the object library targets. If static library targets won't break then that could be the way.

(OT: iotivity builds by CYGWIN and MINGW seem to be broken, but I think I've found the issue; fun for Friday afternoon :D )

If I understand https://cmake.org/cmake/help/latest/policy/CMP0063.html correctly, you can set this policy to NEW and then set the C_VISIBILITY on the object library.

(OT: iotivity builds by CYGWIN and MINGW seem to be broken, but I think I've found the issue; fun for Friday afternoon :D )

I came across this issue as well. One of our Windows developers found that adding this:

#ifdef __MINGW32__ #undef interface #endif

in port/windows/tcpadapter.c and port/windows/network_addresses.c after the include statements fixes the build.

To me, this seems like quite a hack - so I didn't propose the fix upstream. And I also don't know whether CYGWIN and MINGW build are supported by iotivity-lite.

Thanks, that looks indeed relevant. Though I still think that this code is problematic:

#ifdef OC_SHARED_LIBRARY #ifdef OC_SHARED_LIBRARY_EXPORT #define OC_API OC_EXPORT #else /* !OC_SHARED_LIBRARY_EXPORT*/ #define OC_API OC_IMPORT #endif /* OC_SHARED_LIBRARY_EXPORT */ #else /* !OC_SHARED_LIBRARY */ #define OC_API #endif /* OC_SHARED_LIBRARY */

I'm reading about dllexport and if I understand it correctly, whatever the compiler does with the attribute is compiled into the .o file. I see no way around the fact that the .o files are different between static and shared libraries. Shared libraries need OC_SHARED_LIBRARY and OC_SHARED_LIBRARY_EXPORT definitions during compilation. The only way this works is by compiling the object files with those definitions and hoping that a static library doesn't have a problem with that.

I think that it is correct to apply the visibility also on static libraries (btw the current visibility default made all symbols globally visible anyways). So, applying the compile definitions to the object library would promote the correct symbol visibility to the static and the shared library, right?

Though I can think of one side effect, if you compile a static lib from such object files and then link that static lib into a dynamic library, I think the symbols from the static lib will be exposed as global symbols in the final shared library files since they will have the export attribute.

Yes, that's true. I found this question (with an answer) also on stackoverflow: https://stackoverflow.com/questions/2222162/how-to-apply-fvisibility-option-to-symbols-in-static-libraries.

Yeah, seems like we should be able to get both static and shared libraries from object on Unixes. Not sure what will happen on MSVC, but I guess if my testing app links and runs it should be alright ...?

Though if the definitions will be added to both static and shared libraries I should change the names: OC_SHARED_LIBRARY -> OC_LIBRARY and OC_SHARED_LIBRARY_EXPORT -> OC_LIBRARY_EXPORT.

Yeah, seems like we should be able to get both static and shared libraries from object on Unixes. Not sure what will happen on MSVC, but I guess if my testing app links and runs it should be alright ...?

Yes, I think so as well.

Though if the definitions will be added to both static and shared libraries I should change the names: OC_SHARED_LIBRARY -> OC_LIBRARY and OC_SHARED_LIBRARY_EXPORT -> OC_LIBRARY_EXPORT.

Sounds good to me.

It seems to work. Compilation of testing apps that use static lib (uuid-app-static) and shared lib (uuid-app-shared) generated from object lib succeeds on both Linux and Win (see PR jobs). I've also examined uuid-shared with nm -D on Linux and with dumpbin /exports on Win10 locally and the lib only has the manually exposed symbols visible.

Additional notes:

CMP0063 is set to NEW by default, so I skipped setting it explicitly

When compiling a static lib into a shared lib, gcc (at least my 9.3 version) seems to automatically hide all symbols from static library. So one must add compiler flags to make them visible, however this is not an interesting use case currently, so I didn't pursue it further.

OK, so I'll remove the uuid-* testing stuff and we can start annotating public functions and enabling compilation of DLLs on Windows. @WAvdBeek which functions are considered to be public, only those with headers in include/ folder? Any other? (TBH, in my app that links dynamically with libiotivity-client-server.so, I use functions from all over the place, from api, security, port and util folders at least. But I'll work it out.)

include/oc_export.h

Danielius1922 · 2022-02-16T17:32:21Z

(OT: iotivity builds by CYGWIN and MINGW seem to be broken, but I think I've found the issue; fun for Friday afternoon :D )

I came across this issue as well. One of our Windows developers found that adding this:
#ifdef __MINGW32__
#undef interface
#endif
in port/windows/tcpadapter.c and port/windows/network_addresses.c after the include statements fixes the build.

To me, this seems like quite a hack - so I didn't propose the fix upstream. And I also don't know whether CYGWIN and MINGW build are supported by iotivity-lite.

Yes, I spent a good half an hour trying to decipher a weird compilation error on a line with a simple definition of a variable named interface. :D Redefining a common word like interface using a macro seems like a bad idea, but I guess it was necessary for some reason. I would just rename the variables (intf or iface or whatever).

I think at least MINGW was previously supported or intended to be supported, see the cmake-windows workflow:

iotivity-lite/.github/workflows/cmake-windows.yml

Lines 25 to 29 in 88c84f5

    
           #- { 
        
           #    name: "Windows Latest MinGW", artifact: "Windows-MinGW.tar.xz", 
        
           #    os: windows-latest, 
        
           #    build_type: "Release", cc: "gcc", cxx: "g++" 
        
           #  }

I had to do some additional updates to the CMakeLists.txt (if(UNIX) is true in both CYGWIN and MINGW), but after that I was able to make the compilation succeed on both platforms. I think we can get support for both for pretty cheap.

WAvdBeek · 2022-02-16T17:51:58Z

(OT: iotivity builds by CYGWIN and MINGW seem to be broken, but I think I've found the issue; fun for Friday afternoon :D )

I came across this issue as well. One of our Windows developers found that adding this:
#ifdef __MINGW32__
#undef interface
#endif
in port/windows/tcpadapter.c and port/windows/network_addresses.c after the include statements fixes the build.
To me, this seems like quite a hack - so I didn't propose the fix upstream. And I also don't know whether CYGWIN and MINGW build are supported by iotivity-lite.
Yes, I spent a good half an hour trying to decipher a weird compilation error on a line with a simple definition of a variable named interface. :D Redefining a common word like interface using a macro seems like a bad idea, but I guess it was necessary for some reason. I would just rename the variables (intf or iface or whatever).

I think at least MINGW was previously supported or intended to be supported, see the cmake-windows workflow:

iotivity-lite/.github/workflows/cmake-windows.yml

Lines 25 to 29 in 88c84f5

#- {

# name: "Windows Latest MinGW", artifact: "Windows-MinGW.tar.xz",

# os: windows-latest,

# build_type: "Release", cc: "gcc", cxx: "g++"

# }

I had to do some additional updates to the CMakeLists.txt (if(UNIX) is true in both CYGWIN and MINGW), but after that I was able to make the compilation succeed on both platforms. I think we can get support for both for pretty cheap.

nope, this yaml file was just copied from somewhere else and I made it work for visual studio only.
e.g. as precursor of deprecating the visual studio file under port/windows.
maybe the cygwin worked when using the linux port (although I never tested that myself).

Danielius1922 · 2022-02-17T12:12:45Z

nope, this yaml file was just copied from somewhere else and I made it work for visual studio only. e.g. as precursor of deprecating the visual studio file under port/windows. maybe the cygwin worked when using the linux port (although I never tested that myself).

I see, we can get Mingw and Cygwin with some minor code and CMake changes. Though the CMake doesn't run tests, so successful compilation might not mean that everything is in order.

WAvdBeek · 2022-02-22T14:33:38Z

what needs to be done before merge?

Danielius1922 · 2022-02-22T15:43:24Z

what needs to be done before merge?

Well, I've created testing apps (uuid-app, uuid-shared-app) and libraries (uuid-obj, uuid-static, uuid-shared) and I don't think we should merge those into master. However, those are an example of how to do it, so we should create at least one library target with static and shared versions that work. I guess the client library is the smallest? We need to annotate all public functions with the OC_API macro, I can start doing it, but I'm not sure which functions exactly are considered public. Only those in the include/ folder? Any others?

WAvdBeek · 2022-02-22T15:46:22Z

what needs to be done before merge?

Well, I've created testing apps (uuid-app, uuid-shared-app) and libraries (uuid-obj, uuid-static, uuid-shared) and I don't think we should merge those into master. However, those are an example of how to do it, so we should create at least one library target with static and shared versions that work. I guess the client library is the smallest? We need to annotate all public functions with the OC_API macro, I can start doing it, but I'm not sure which functions exactly are considered public. Only those in the include/ folder? Any others?

oc_python.h, so that we can use the python code on windows. For me that would be enough for a merge.

Danielius1922 · 2022-02-22T17:10:10Z

oc_python.h, so that we can use the python code on windows. For me that would be enough for a merge.

I've removed the testing apps and iotivity-lite-client-python.dll compiled on Windows / libiotivity-lite-client-python.so on Linux, though on Linux symbols from common-obj, tinycbor-master, client-obj and mbedtls are exported as part of the lib. I think I can hide all symbols from targets mbedtls and tinycbor-master, those are third-party libs, no reason to expose their symbols as part of any iotivity library. For targets common-obj and client-obj I think some functions should be exported.

WAvdBeek · 2022-02-23T09:16:30Z

oc_python.h, so that we can use the python code on windows. For me that would be enough for a merge.

I've removed the testing apps and iotivity-lite-client-python.dll compiled on Windows / libiotivity-lite-client-python.so on Linux, though on Linux symbols from common-obj, tinycbor-master, client-obj and mbedtls are exported as part of the lib. I think I can hide all symbols from targets mbedtls and tinycbor-master, those are third-party libs, no reason to expose their symbols as part of any iotivity library. For targets common-obj and client-obj I think some functions should be exported.

not sure if it is an actual issue that those symbols are exported. those symbols are used internally in the shared object/dll anyway.

Danielius1922 · 2022-02-23T09:58:22Z

not sure if it is an actual issue that those symbols are exported. those symbols are used internally in the shared object/dll anyway.

Yes, that's not a problem that the shared library uses those symbols internally, the issue is that the shared library exports those symbols (at least on Unixes). That is they are available to a binary that links with the shared library.
The default behavior is different on MSVC and on GCC-compatible compilers (gcc, clang, mingw, cygwin). MSVC hides symbols by default, so global function from those mentioned targets are not visible to the clients of the .dll. GCC on other hand exposes symbols by default, so all global functions from the mentioned targets are visible to the clients of the .so.

Take the client-python-shared which is now built by this CMake on both Windows and on Unix:

for Windows (with MSVC): the .dll only exposes functions annotated in the oc_python.h
for Unix: the .so exposes functions from oc_python.h, but also all global functions from common-obj, tinycbor-master,
client-obj, mbedtls.

I think we should have same or at least very similar exposed API on all platforms, so additional work is needed. But oc_python.c stuff is workable now, so I suggest we merge this PR and the remaining work will be done later.

Danielius1922 force-pushed the adam/feature/add-exports branch from 8dda993 to fa98b95 Compare February 11, 2022 10:07

Danielius1922 requested review from jkralik, Oxymoron79 and WAvdBeek February 11, 2022 11:21

WAvdBeek approved these changes Feb 11, 2022

View reviewed changes

Danielius1922 commented Feb 11, 2022

View reviewed changes

include/oc_export.h Outdated Show resolved Hide resolved

Danielius1922 commented Feb 11, 2022

View reviewed changes

Danielius1922 force-pushed the adam/feature/add-exports branch 2 times, most recently from 5cb2977 to 6e9560d Compare February 11, 2022 18:02

Oxymoron79 reviewed Feb 11, 2022

View reviewed changes

include/oc_export.h Outdated Show resolved Hide resolved

Oxymoron79 reviewed Feb 11, 2022

View reviewed changes

include/oc_export.h Outdated Show resolved Hide resolved

Danielius1922 force-pushed the adam/feature/add-exports branch 4 times, most recently from 506253e to 4bea4ae Compare February 15, 2022 08:23

Danielius1922 force-pushed the adam/feature/add-exports branch from 4bea4ae to 2473c45 Compare February 18, 2022 09:55

WAvdBeek approved these changes Feb 22, 2022

View reviewed changes

Danielius1922 force-pushed the adam/feature/add-exports branch from 2473c45 to e734521 Compare February 22, 2022 16:39

Add support for shared libraries on Windows

3cde7a0

Danielius1922 force-pushed the adam/feature/add-exports branch from e734521 to 3cde7a0 Compare February 22, 2022 16:42

Danielius1922 marked this pull request as ready for review February 23, 2022 09:58

WAvdBeek merged commit 3f760f2 into master Feb 23, 2022

github-actions bot locked and limited conversation to collaborators Feb 23, 2022

Danielius1922 deleted the adam/feature/add-exports branch September 8, 2022 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test shared library on Windows #186

Test shared library on Windows #186

Danielius1922 commented Feb 11, 2022

Danielius1922 commented Feb 11, 2022 •

edited

Loading

WAvdBeek commented Feb 11, 2022

Danielius1922 commented Feb 11, 2022 •

edited

Loading

WAvdBeek commented Feb 11, 2022

Danielius1922 commented Feb 11, 2022 •

edited

Loading

Danielius1922 Feb 11, 2022

Danielius1922 Feb 14, 2022

Oxymoron79 Feb 14, 2022

Danielius1922 Feb 15, 2022

Oxymoron79 Feb 15, 2022

Oxymoron79 Feb 16, 2022

Oxymoron79 Feb 16, 2022

Danielius1922 Feb 17, 2022 •

edited

Loading

Oxymoron79 Feb 17, 2022

Danielius1922 Feb 18, 2022

Danielius1922 commented Feb 16, 2022

WAvdBeek commented Feb 16, 2022

Danielius1922 commented Feb 17, 2022

WAvdBeek commented Feb 22, 2022

Danielius1922 commented Feb 22, 2022

WAvdBeek commented Feb 22, 2022

Danielius1922 commented Feb 22, 2022

WAvdBeek commented Feb 23, 2022

Danielius1922 commented Feb 23, 2022

Test shared library on Windows #186

Test shared library on Windows #186

Conversation

Danielius1922 commented Feb 11, 2022

Danielius1922 commented Feb 11, 2022 • edited Loading

WAvdBeek commented Feb 11, 2022

Danielius1922 commented Feb 11, 2022 • edited Loading

WAvdBeek commented Feb 11, 2022

Danielius1922 commented Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danielius1922 Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danielius1922 commented Feb 16, 2022

WAvdBeek commented Feb 16, 2022

Danielius1922 commented Feb 17, 2022

WAvdBeek commented Feb 22, 2022

Danielius1922 commented Feb 22, 2022

WAvdBeek commented Feb 22, 2022

Danielius1922 commented Feb 22, 2022

WAvdBeek commented Feb 23, 2022

Danielius1922 commented Feb 23, 2022

Danielius1922 commented Feb 11, 2022 •

edited

Loading

Danielius1922 commented Feb 11, 2022 •

edited

Loading

Danielius1922 commented Feb 11, 2022 •

edited

Loading

Danielius1922 Feb 17, 2022 •

edited

Loading